Fix sending heroes in SSS when m.room.name=""#19468
Fix sending heroes in SSS when m.room.name=""#19468frebib wants to merge 1 commit intoelement-hq:developfrom
m.room.name=""#19468Conversation
|
|
frebib
force-pushed
the
frebib/fix-19447
branch
2 times, most recently
from
5792708 to
bf1daac
Compare
|
Confirmed that this fixes the issue for me, although I'd like some input on exactly how to implement this, perhaps. I fear that the extra round-trip(s) to the database on every sync request aren't the most optimal way to implement this |
As per the spec, a room with m.room.name value that is absent, null or empty should be treated as if there is no m.room.name event at all: https://spec.matrix.org/v1.17/client-server-api/#mroomname This fetches the full m.room.name event and checks the content.name instead of only checking the existence of the m.room.name event. This results in correctly sending heroes for those rooms. Fixes: element-hq#19447 Signed-off-by: Joe Groocock <me@frebib.net>
frebib
force-pushed
the
frebib/fix-19447
branch
from
bf1daac to
acf5ada
Compare
| ] | ||
| meta_room_state = list(hero_room_state) | ||
| if initial or name_changed: | ||
| meta_room_state.append((EventTypes.Name, "")) |
There was a problem hiding this comment.
It's possible that this isn't needed now. I wasn't sure whether it was safe to remove or not though
reivilibre
left a comment
reivilibre
left a comment
There was a problem hiding this comment.
Really sorry for the long wait.
This bug fix seems correct to me, so thanks for sending it in.
If you're happy to take a look, I'd be grateful for the tweak to avoid this op when it's not needed.
|
|
||
| # TODO: Should we also check for `EventTypes.CanonicalAlias` | ||
| # (`m.room.canonical_alias`) as a fallback for the room name? see | ||
| # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1671260153 |
There was a problem hiding this comment.
can also link to https://github.com/matrix-org/matrix-spec-proposals/pull/4186/changes#r2860107511 which I just opened as an equivalent thread on the non-obsolete MSC.
I think you're currently correct in not using the canonical alias
There was a problem hiding this comment.
(I just realised this is not your comment, you just moved it, sorry & thanks)
There was a problem hiding this comment.
Sure, I'll update the comment link
| @@ -0,0 +1 @@ | |||
| Fix bug in sync that could prevent user avatars from showing if the room had an empty name. | |||
There was a problem hiding this comment.
| Fix bug in sync that could prevent user avatars from showing if the room had an empty name. | |
| Fix a bug in [MSC4186: Simplified Sliding Sync](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) that could prevent user avatars from showing if the room had an empty name. |
I know it's nitpicky but useful for people reading to understand this is not oldschool sync.
| membership_changed = False | ||
| name_changed = False | ||
| avatar_changed = False | ||
| if initial: |
There was a problem hiding this comment.
hmm, sorry to be a pest but I think it'd be nice to keep the performance properties of the old code:
we didn't get the m.room.name state unless the sync was initial.
I think this is getting run once per room on every sync so it feels important enough to at least try.
Instead of name_event_id: str | None how about pulling out the full event name_event: EventBase | None
I suspect we want to retrieve the name_event in two conditions:
initial- or
name_changed(L908 / R899)
Also thinking that we need to properly handle the case where the room name is initially "My Room" and then someone changes it to "" at which point we need to start using heroes.
There was a problem hiding this comment.
I'm all for keeping the performance here if at all possible. The code is rather complex and I'm scared of introducing a regression 😆
Also thinking that we need to properly handle the case where the room name is initially
"My Room"and then someone changes it to""at which point we need to start using heroes.
Given that sync is stateless, I'm not sure if there's a way to do this without unconditionally fetching this event every time to look at the content to discern whether we need to send heroes or not.
4 participants
As per the spec, a room with m.room.name value that is absent, null or empty should be treated as if there is no m.room.name event at all: https://spec.matrix.org/v1.17/client-server-api/#mroomname
This fetches the full m.room.name event and checks the content.name instead of only checking the existence of the m.room.name event. This results in correctly sending heroes for those rooms.
Fixes: #19447
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.